I wonder if we should limit the scope to change between two timesteps and binary change - then we can use binary metrics and provide a template for the plot methods. I say this because this is the most common change detection task by a mile. Might also simplify the augmentations approach? Treating as a video sequence seems overkill.
Also I understand there is support for multitemporal coming later.

I wonder if we should limit the scope to change between two timesteps

I'm personally okay with this, although @hfangcat has a recent work using multiple pre-event images that would be nice to support someday (could be a subclass if necessary).

and binary change

Again, this would probably be fine as a starting point, although I would someday like to make all trainers support binary/multiclass/multilabel, e.g., #2219.

provide a template for the plot methods.

Could also do this in the datasets (at least for benchmark NonGeoDatasets). We're also trying to remove explicit plotting in the trainers: #2184

I say this because this is the most common change detection task by a mile.

Agreed.

Might also simplify the augmentations approach? Treating as a video sequence seems overkill.

I actually like the video augmentations, but let me loop in the Kornia folks to get their opinion: @edgarriba @johnnv1

Also I understand there is support for multitemporal coming later.

Correct, see #2382 for the big picture (I think I also sent you a recording of my presented plan).

VideoSequential adds a temporal dimension to the mask. Not sure if there is a way to avoid this

Can you try keepdim=True?

I added an if statement to the AugmentationSequential wrapper to check for and remove this added dimension.

@ashnair1 would this work directly with K.AugmentationSequential now? We are trying to phase out our AugmentationSequential wrapper now that upstream supports (almost?) everything we need.

I will go ahead and make changes for this to be for binary change and two timesteps, sounds like a good starting point.

Can you try keepdim=True?

I tried this and it didn't get rid of the other dimension. I also looked into extra_args but didn't see any options to help with this.

Could also do this in the datasets (at least for benchmark NonGeoDatasets). We're also trying to remove explicit plotting in the trainers

I was going to add plotting in the trainer, but would you rather not then? What would this look like in the dataset?

Perhaps there should even be a base class ChangeDetection and subclasses for BinaryChangeDetection etc?

That's exactly what I'm trying to undo in #2219.

I was going to add plotting in the trainer, but would you rather not then?

We can copy-n-paste the validation_step plotting stuff used by other trainers, but that's probably going to disappear soon (I think we're just waiting on testing in #2184.

What would this look like in the dataset?

See OSCD.plot()

I've updated this to now support only binary change with two timesteps.
To get test_weight_file in test_change.py to work with the two images stacked on the channel dimension for Unet, I modified the pytest fixture model() in conftest.py to use timm to create the model instead of torchvision, so that an in_channels parameter can be passed.

I still haven't been able to figure out how to make transforms.transforms._RandomNCrop work with the added temporal dimension. It seems to have something to do with _NCropGenerator not properly handling the temporal dimension but I really don't understand what is going on there.

I'm going to need some help figuring out how to get transforms.transforms._RandomNCrop to work with the added temporal dimension (this is used by the OSCD dataset). I've delved into this a few times but with my lack of familiarity with Kornia I haven't been able to track down the source of the issue. You can see the issue by running tests/trainers/test_change.py::TestChangeDetectionTask::test_trainer.

Also, disregard my earlier comments about Kornia VideoSequential adding a dimension to the mask, this seems to have resolved with the latest Kornia version.

I see that in the automated pytest checks (in tests / minimum) there was a syntax error, but I don't see this issue locally. How do I resolve this?

Couldn't reproduce either, might be a bug specific to older Python versions. Anyway, undid the change on the line it was complaining about, let's see if that fixes it.

I noticed I also need to update tests/datasets/test_oscd.py. And I think we are removing tests/datamodules/test_oscd.py per #978 now that we have a change detection task, right?

Also, how do you usually run prettier locally? I see there is an issue in the prettier check here but it isn't installed in the devcontainer unless I'm mistaken.

I think we are removing tests/datamodules/test_oscd.py per #978 now that we have a change detection task, right?

Correct.

Also, how do you usually run prettier locally? I see there is an issue in the prettier check here but it isn't installed in the devcontainer unless I'm mistaken.

To be honest, I rarely run prettier locally. But here are the docs on how to do it: https://torchgeo.readthedocs.io/en/latest/user/contributing.html#linters

I've also never used the devcontainer before. But it looks like there is a VS Code extension for prettier: https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode. Feel free to add that to the devcontainer in a separate PR if you want.

How hard would it be to do late fusion, so pass each image through the encoder separately, then concatenate them, then pass them through the decoder?

@adamjstewart I think this is a remaining question to resolve on this PR, and I realized that this is already what torchgeo.models.FCSiamConc is doing, right? And FCSiamConc is one of the model options for this trainer. So maybe it makes sense to also have an option that concatenates the images before passing through the model (keeping the Unet how it is).

@isaaccorley and @adamjstewart I think this is ready. Let me know what I should put in ChangeDetectionTask for versionadded.

SemanticSegmentationTask, on which this trainer is based, changed a lot in #2560, #2219, and #2690. We should update this PR with those same changes.

I could add the denormalization for plotting, but as far as adding support for multiclass and multilabel, we specifically decided that this PR would just support binary change detection, so I'd like to keep it like that and someone else can extend it to those cases later.

SemanticSegmentationTask, on which this trainer is based, changed a lot in #2560, #2219, and #2690. We should update this PR with those same changes.

I could add the denormalization for plotting, but as far as adding support for multiclass and multilabel, we specifically decided that this PR would just support binary change detection, so I'd like to keep it like that and someone else can extend it to those cases later.

I could have a look at adding multiclass & multilabel support once this PR is merged. As I have done something similar already for my research. If you @adamjstewart would guide me a bit for my first PR to torchgeo on this one?

@keves1 yes, let's add denormalizing and switch from if-statements to match-statements.

I'll let @hkristen take a stab at non-binary change detection in a follow-up PR after this is merged. I know @hfangcat is also interested in change detection for 3+ images, I'm not sure if that would be a feature to add to ChangeDetectionTask or SemanticSegmentationTask. I would like to merge this PR soon as I think it's almost ready, we can iterate on additional features later.

@adamjstewart I added denormalizing and switched to match statements, as well as made the other changes you suggested. I think it's ready now.

Looks like the way I did the monkeypatch to test predict_step when there isn't a predict dataset didn't work (it didn't run predict_step). What would be the right way to do this? I tried setting predict_dataloader and predict_dataset.

Very close to being ready to merge, excited to finally get this in!!!

@adamjstewart Let me know if this looks ready now!

@hkristen now that this is merged, you can take a stab at adding multiclass/multilabel support if you want. See #2219 for what this change looked like for SemanticSegmentationTask (code should be almost identical). Also see https://torchgeo.readthedocs.io/en/latest/user/contributing.html for general contributing guidelines. Happy to help with any questions you have or tests you encounter trouble with.

Thanks for the hard work on this, so happy to finally merge!

You're welcome, thanks for your help, and glad it could be merged!